Flatbuffers impl #446

boocmp · 2025-03-28T08:24:44Z

The NetworkFilterList implementation is now based on FlatBuffers.
The serialization format has been updated to support this change.
Serialization benchmarks have been moved to a separate file.
Support for adding filters after NetworkFilterList creation has been removed. Since rebuilding the entire FlatBuffer is costly, this functionality may be reintroduced in the future if there is a clear need for it.

github-actions

Rust Benchmark

Benchmark suite	Current: `537ab54`	Previous: `67b7b70`	Ratio
`rule-match-browserlike/brave-list`	`2150693604` ns/iter (`± 16942629`)	`1803081237` ns/iter (`± 16195247`)	`1.19`
`rule-match-first-request/brave-list`	`1017455` ns/iter (`± 36469`)	`980016` ns/iter (`± 5452`)	`1.04`
`blocker_new/brave-list`	`158380700` ns/iter (`± 1023726`)	`203961371` ns/iter (`± 2004867`)	`0.78`
`memory-usage/brave-list-initial`	`21536659` ns/iter (`± 3`)	`41762172` ns/iter (`± 3`)	`0.52`
`memory-usage/brave-list-after-1000-requests`	`24141128` ns/iter (`± 3`)	`44355700` ns/iter (`± 3`)	`0.54`

This comment was automatically generated by workflow using github-action-benchmark.

github-actions · 2025-04-09T00:58:13Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{reported by reviewdog 🐶}
[semgrep] Detected 'unsafe' usage, please audit for secure usage

Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage

Cc @thypon @kdenhartog

It turns out that from_raw_parts results in issues if bytes.as_ptr() isn't aligned to 2 bytes We need to assert this.

assert added

github-actions · 2025-04-09T00:58:13Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{reported by reviewdog 🐶}
[semgrep] Detected 'unsafe' usage, please audit for secure usage

Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage

Cc @thypon @kdenhartog

github-actions · 2025-04-09T00:58:13Z

src/flat_network_filter_list.rs

+        }
+
+        let filters_list =
+            unsafe { fb::root_as_network_filter_list_unchecked(&self.flatbuffer_memory) };


_{reported by reviewdog 🐶}
[semgrep] Detected 'unsafe' usage, please audit for secure usage

Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage

Cc @thypon @kdenhartog

@boocmp use root_as_network_filter_list() + .expect() to remove unsafe

it runs a verification process, it'll degrade performance.

Until we build the flatbuffer just before using root_as_network_filter_list_unchecked should be fine in terms of security.

added this check on deserialization

github-actions · 2025-04-09T00:58:13Z

src/flatbuffers/fb_network_filter_generated.rs

+            unsafe {
+                self._tab
+                    .get::<flatbuffers::ForwardsUOffset<&str>>(NetworkFilter::VT_RAW_LINE, None)
+            }


_{reported by reviewdog 🐶}
[semgrep] Detected 'unsafe' usage, please audit for secure usage

Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage

Cc @thypon @kdenhartog

atuchin-m

LGTM with a few nits

src/filters/fb_network.rs

.github/workflows/perf-ci.yml

src/filters/fb_network.rs

github-actions · 2025-04-10T11:28:44Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{reported by reviewdog 🐶}
[semgrep] Detected 'unsafe' usage, please audit for secure usage

Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage

Cc @thypon @kdenhartog

github-actions · 2025-04-10T11:28:44Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{reported by reviewdog 🐶}
[semgrep] Detected 'unsafe' usage, please audit for secure usage

Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage

Cc @thypon @kdenhartog

src/blocker.rs

antonok-edm · 2025-04-17T18:30:17Z

src/blocker.rs

+        let mut disabled_directives: HashSet<String> = HashSet::new();
+        let mut enabled_directives: HashSet<String> = HashSet::new();


why not &str anymore?

returned &str back

src/data_format/mod.rs

src/engine_serializer.rs

src/filters/fb_network.rs

github-actions · 2025-04-28T07:11:37Z

src/network_filter_list.rs

        if self.filter_map.is_empty() {
            return None;
        }

+        let filters_list =
+            unsafe { fb::root_as_network_filter_list_unchecked(&self.flatbuffer_memory) };


_{reported by reviewdog 🐶}
[semgrep] Detected 'unsafe' usage, please audit for secure usage

Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage

Cc @thypon @kdenhartog

github-actions · 2025-04-28T07:11:37Z

src/network_filter_list.rs


        if self.filter_map.is_empty() {
            return filters;
        }

+        let filters_list =
+            unsafe { fb::root_as_network_filter_list_unchecked(&self.flatbuffer_memory) };


_{reported by reviewdog 🐶}
[semgrep] Detected 'unsafe' usage, please audit for secure usage

Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage

Cc @thypon @kdenhartog

github-actions · 2025-04-28T08:02:49Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{reported by reviewdog 🐶}
[semgrep] Detected 'unsafe' usage, please audit for secure usage

Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage

Cc @thypon @kdenhartog

github-actions · 2025-04-28T08:02:49Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{reported by reviewdog 🐶}
[semgrep] Detected 'unsafe' usage, please audit for secure usage

Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage

Cc @thypon @kdenhartog

github-actions · 2025-04-29T04:33:41Z

src/filters/fb_network.rs

+                    bytes.as_ptr() as *const u16,
+                    bytes.len() / std::mem::size_of::<u16>(),
+                )
+            }


_{reported by reviewdog 🐶}
[semgrep] Detected 'unsafe' usage, please audit for secure usage

Source: https://semgrep.dev/r/rust.lang.security.unsafe-usage.unsafe-usage

Cc @thypon @kdenhartog

src/engine_serializer.rs

antonok-edm · 2025-05-13T16:45:10Z

src/filters/fb_network.rs

+        index: u32,
+        owner: &'a NetworkFilterList,
+    ) -> Self {
+        let list_address: *const NetworkFilterList = owner as *const NetworkFilterList;


pointer casting often indicates some problematic design patterns in Rust. I think we can remove it in this instance?

this owner field can increase memory usage since it's an additional address-sized field on each filter, even though they'd all have the same value for all filters in a list

I only see it used in NetworkMatchable to make it conform to the existing API. I'm not opposed to that API changing if needed.

FlatNetworkFilter is a wrapper over the fb::NetworkFilter. We only create the instance of it when found the the fb::NetworkFilter in the NetworkFilterList so it does't increase the memory usage because it temporary object on the stack. For now, regex manager is designed to use unique key to find corresponding regex, previously it was an address of the NetworkFilter, but now we don't have unique address for every filter because they stored in flatbuffer. To create unique key we use the list address combined with the filter's index. I agree it looks ugly but I don't know how to do it better.

Let's understand why we need this unique key.
If it's only for regex manager, let's:

try to move key calculation here

add TODO to make keys unique only in scope of regex manager. I believe once that in one of the next steps we move from multiple flatbuffers to a just one that ables to contain multiple lists. That way we get 1:1 proportion (regex manager: flatbuffer).

Also, this could be done in follow up: this PR is already waiting for a long time.

1 regex manager: 1 flatbuffer definitely sounds like the right way to do it, but agreed it can be followup if needed

src/filters/fb_network.rs

antonok-edm · 2025-05-13T16:51:12Z

src/network_filter_list.rs

+#[derive(Serialize, Deserialize)]
+pub struct NetworkFilterList {
+    pub(crate) flatbuffer_memory: Vec<u8>,
+    pub(crate) filter_map: HashMap<Hash, Vec<u32>>,
+    pub(crate) unique_domains_hashes_map: HashMap<Hash, u16>,
+}
+
+impl Default for NetworkFilterList {
+    fn default() -> Self {
+        Self {
+            flatbuffer_memory: Default::default(),
+            filter_map: Default::default(),
+            unique_domains_hashes_map: Default::default(),
+        }
+    }


#[derive(Serialize, Deserialize, Default)] should automatically produce this Default implementation already

For some reason it doesn't, I think because of the new version of serde

Oh,,god, I forgot to add Default it the derive list... I'll fix it in one of the follow ups

src/network_filter_list.rs

tests/unit/blocker.rs

src/data_format/v0.rs

Added raw_line field to flatbuffers for testing.

antonok-edm

the other thing left unaddressed is #446 (comment), still not sure if it's required

antonok-edm · 2025-05-19T05:09:52Z

src/data_format/mod.rs


-use crate::blocker::Blocker;
 use crate::cosmetic_filter_cache::CosmeticFilterCache;

+use crate::blocker::Blocker;
+


nit but I don't understand the reason to move Blocker into a separate paragraph by itself (similar change also in src/data_format/v0.rs)

github-actions · 2025-05-19T06:00:11Z

[puLL-Merge] - brave/adblock-rust@446

Description

This PR changes how serialization works in the adblock-rust library by implementing a FlatBuffers-based storage for network filters. The main motivations are to improve performance and reduce memory usage by replacing the old serialization system with a more efficient FlatBuffers approach. The PR also reorganizes the benchmarking structure by moving serialization benchmarks to a separate file.

Changes

Changes

Cargo.toml
- Changed flatbuffers dependency from optional to required
- Removed flatbuffers-storage feature flag
- Added a new benchmark for serialization
Benchmarking
- Moved serialization benchmarks from bench_matching.rs to a new file bench_serialization.rs
- Updated benchmark code to call serialize() instead of serialize_raw()
Engine API
- Renamed serialize_raw() to serialize()
- Updated deserialize() method to handle FlatBuffers format
- Updated all references to these methods throughout the codebase
FlatBuffers Implementation
- Added a new module fb_network.rs with FlatBuffers implementation
- Enhanced the FlatBuffers schema to include more fields
- Implemented efficient lookups using domain hash maps
Network Filter List
- Completely refactored the NetworkFilterList to use FlatBuffers storage
- Changed data structures to use integer indices instead of Arc pointers
- Implemented optimizations for domain lookups
Data Format
- Updated serialization/deserialization to handle FlatBuffers format
- Added proper error handling for FlatBuffers deserialization
Blocker
- Simplified the blocker code by removing direct filter manipulation methods
- Changed how CSP directives are handled to be more efficient
Tests
- Updated tests to work with the new serialization format
- Removed tests for removed functionality (like add_filter)

sequenceDiagram
    participant App as Application
    participant Engine as Engine
    participant Serializer as Serializer
    participant FB as FlatBuffers
    participant NetworkList as NetworkFilterList
    
    App->>Engine: serialize()
    Engine->>Serializer: SerializeFormat::build()
    Serializer->>FB: create FlatBuffersBuilder
    
    loop For each network filter
        FB->>FB: add filter to buffer
        FB->>NetworkList: track domain hash indices
    end
    
    FB->>Serializer: finish buffer
    Serializer->>Engine: return serialized data
    Engine->>App: return Vec<u8>
    
    App->>Engine: deserialize(data)
    Engine->>FB: parse FlatBuffer data
    FB->>NetworkList: rebuild filter map
    FB->>NetworkList: rebuild domain hash maps
    NetworkList->>Engine: return deserialized structures
    Engine->>App: Ok(())

Possible Issues

The PR removes some public APIs like Blocker::add_filter() and Blocker::optimize() which might break backward compatibility for users who directly manipulate the blocker.
String handling for CSP directives was changed from references to owned strings, which could potentially impact performance if these are frequently accessed.
Test serialization data was completely changed, suggesting that the new format is incompatible with previously serialized data.

github-actions bot reviewed Mar 28, 2025

View reviewed changes

boocmp force-pushed the flatbuffers_impl branch from 91c5ec7 to cacfcc6 Compare April 9, 2025 00:55

github-actions bot reviewed Apr 9, 2025

View reviewed changes

github-actions bot added the needs-security-review label Apr 9, 2025

github-actions bot assigned kdenhartog and thypon Apr 9, 2025

boocmp force-pushed the flatbuffers_impl branch from cacfcc6 to a3df20d Compare April 9, 2025 01:05

boocmp marked this pull request as ready for review April 9, 2025 01:38

boocmp requested review from atuchin-m and antonok-edm April 9, 2025 01:38

atuchin-m approved these changes Apr 9, 2025

View reviewed changes

boocmp force-pushed the flatbuffers_impl branch from 6a55c00 to a3df20d Compare April 9, 2025 11:30

github-actions bot added the puLL-Merge label Apr 9, 2025

github-actions bot reviewed Apr 10, 2025

View reviewed changes

antonok-edm reviewed Apr 18, 2025

View reviewed changes

github-actions bot reviewed Apr 28, 2025

View reviewed changes

boocmp force-pushed the flatbuffers_impl branch 4 times, most recently from 0238db7 to a40fa5e Compare April 28, 2025 07:50

github-actions bot reviewed Apr 28, 2025

View reviewed changes

github-actions bot reviewed Apr 29, 2025

View reviewed changes

antonok-edm reviewed May 13, 2025

View reviewed changes

boocmp requested review from antonok-edm and atuchin-m May 16, 2025 08:00

atuchin-m reviewed May 16, 2025

View reviewed changes

src/data_format/v0.rs Outdated Show resolved Hide resolved

atuchin-m approved these changes May 16, 2025

View reviewed changes

boocmp added 3 commits May 16, 2025 18:38

Added NetworkFilterListTrait to futher provide the flatbuffer impl.

76bedcf

Added FlatNetworkFilterList implementation.

0860f67

Added raw_line field to flatbuffers for testing.

Blocker type for data_format lib.

7f86186

boocmp added 11 commits May 16, 2025 18:38

Added EngineSerializer trait

949f816

Renaming.

05db953

Review.

9fb57fc

NetworkFilterList impl replaced with flatbuffers.

b87f0f6

Review.

dd775dc

Added checks for flatbuffers structures.

da6a0ee

Removed BlockerError.

bf03647

pub(crate) for NetworkFilterList.

730695d

Shorthand init.

56202a7

Removed EngineSerializer trait.

3c816ca

Deserialise returns Result<...> now.

79b0b83

boocmp force-pushed the flatbuffers_impl branch from a541821 to 79b0b83 Compare May 16, 2025 11:38

antonok-edm approved these changes May 19, 2025

View reviewed changes

Clean up.

b803db3

String -> &str

537ab54

boocmp force-pushed the flatbuffers_impl branch from 4a1d750 to 537ab54 Compare May 19, 2025 06:23

boocmp merged commit 50548c4 into master May 19, 2025
8 checks passed

boocmp deleted the flatbuffers_impl branch May 19, 2025 08:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flatbuffers impl #446

Flatbuffers impl #446

boocmp commented Mar 28, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading

github-actions bot Apr 9, 2025

atuchin-m Apr 9, 2025

boocmp Apr 29, 2025

github-actions bot Apr 9, 2025

github-actions bot Apr 9, 2025

atuchin-m Apr 9, 2025

boocmp Apr 28, 2025

atuchin-m Apr 28, 2025

boocmp Apr 29, 2025

github-actions bot Apr 9, 2025

atuchin-m left a comment

github-actions bot Apr 10, 2025

github-actions bot Apr 10, 2025

antonok-edm Apr 17, 2025

boocmp May 19, 2025

github-actions bot Apr 28, 2025

github-actions bot Apr 28, 2025

github-actions bot Apr 28, 2025

github-actions bot Apr 28, 2025

github-actions bot Apr 29, 2025

antonok-edm May 13, 2025

boocmp May 16, 2025

atuchin-m May 16, 2025

atuchin-m May 16, 2025

antonok-edm May 19, 2025

antonok-edm May 13, 2025

boocmp May 16, 2025

boocmp May 19, 2025

antonok-edm left a comment

antonok-edm May 19, 2025

github-actions bot commented May 19, 2025

Changes

		let mut disabled_directives: HashSet<String> = HashSet::new();
		let mut enabled_directives: HashSet<String> = HashSet::new();

Flatbuffers impl #446

Flatbuffers impl #446

Conversation

boocmp commented Mar 28, 2025 • edited Loading

github-actions bot left a comment • edited Loading

Choose a reason for hiding this comment

Rust Benchmark

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Apr 9, 2025

Choose a reason for hiding this comment

atuchin-m left a comment

Choose a reason for hiding this comment

github-actions bot Apr 10, 2025

Choose a reason for hiding this comment

github-actions bot Apr 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot Apr 28, 2025

Choose a reason for hiding this comment

github-actions bot Apr 28, 2025

Choose a reason for hiding this comment

github-actions bot Apr 28, 2025

Choose a reason for hiding this comment

github-actions bot Apr 28, 2025

Choose a reason for hiding this comment

github-actions bot Apr 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonok-edm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 19, 2025

Description

Changes

Possible Issues

boocmp commented Mar 28, 2025 •

edited

Loading

github-actions bot left a comment •

edited

Loading